Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a HeaderMap to HttpError #1193

Merged
merged 8 commits into from
Dec 4, 2024
Merged

Add a HeaderMap to HttpError #1193

merged 8 commits into from
Dec 4, 2024

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Dec 4, 2024

There are certain response headers which the HTTP standard specifies
that servers should send along with particular error responses or status
codes. For example, when returning a 405 Method Not Allowed status,
§ 15.5.0 RFC9110 states that:

The origin server MUST generate an Allow header field in a 405
response containing a list of the target resource's currently
supported methods.

Currently, Dropshot does not do this, so we are technically not
compliant with RFC9110.

Similarly, some of Dropshot's extractors return error responses for
which we ought to emit headers. For example, the TypedBody extractor
will return a 415 Unsupported Media Type status, for which § 15.5.16
RFC9110
recommends returning an Accept or Accept-Encoding header
as appropriate:

If the problem was caused by an unsupported content coding, the
Accept-Encoding response header field (Section 12.5.3) ought to be
used to indicate which (if any) content codings would have been
accepted in the request.

On the other hand, if the cause was an unsupported media type, the
Accept response header field (Section 12.5.1) can be used to indicate
which media types would have been accepted in the request.

Note that, unlike the Allow header for 405 responses, this is just a
suggestion --- the RFC doesn't include normative language stating that
we MUST do this. But it's nice if we do.

Errors emitted internally by Dropshot are represented by the HttpError
type. Currently, there is no way for this type to provide headers which
should be added to responses generated from an HttpError. Therefore,
this branch does so by adding a http::HeaderMap to HttpError. When
the HttpError is converted into a response, any headers in the
HeaderMap are added to the response. Adding this field is a breaking
change, so I've added it to the changelog.

Because the http::HeaderMap type is fairly large even when empty, the
HeaderMap is stored in an Option<Box<HeaderMap>> to reduce the size
of HttpErrors, especially in the common case where there is no header
map to add. Unlike containers such as Vec and HashMap, an empty
http::HeaderMap contains a fairly large number of fields, rather than
being a single pointer to a heap allocation containing the actual data.
Thus, we store an Option<Box<HeaderMap>> here, so that the empty case
is just a null pointer. Not doing this results in a clippy warning
on...every function returning a Result<_, HttpError>, which is
unfortunate because even if we ignored that lint in Dropshot, it would
still trigger for consumers of Dropshot's APIs.

As an initial proof of concept, I've added code to dropshot's router
module to add Allow headers with the list of allowed methods when
returning a 405 Method Not Allowed error. There are other places where
we should be returning headers in our error responses, but this felt
like one of the more important ones, since the RFC says we MUST do it.
We can add headers to other errors separately; my priority was to make
the breaking parts of the change first.

For example, note that we now return Allow headers in responses to
requests with invalid methods:

eliza@noctis ~/Code/dropshot $ cargo run --example basic &
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/examples/basic`
Dec 04 18:51:47.826 INFO listening, local_addr: 127.0.0.1:41751

eliza@noctis ~/Code/dropshot $ curl -v http://localhost:41751/counter -X DELETE
* Host localhost:41751 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:41751...
* connect to ::1 port 41751 from ::1 port 53992 failed: Connection refused
*   Trying 127.0.0.1:41751...
* Connected to localhost (127.0.0.1) port 41751
* using HTTP/1.x
> DELETE /counter HTTP/1.1
> Host: localhost:41751
> User-Agent: curl/8.11.0
> Accept: */*
>
* Request completely sent off
Dec 04 18:59:14.913 INFO accepted connection, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751
Dec 04 18:59:14.917 INFO request completed, error_message_external: Method Not Allowed, error_message_internal: Method Not Allowed, latency_us: 834, response_code: 405, uri: /counter, method: DELETE, req_id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751
< HTTP/1.1 405 Method Not Allowed
< allow: GET
< allow: PUT
< content-type: application/json
< x-request-id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793
< content-length: 93
< date: Wed, 04 Dec 2024 18:59:14 GMT
<
{
  "request_id": "e1d14d57-ca67-49cc-84b0-d1dc1e0e5793",
  "message": "Method Not Allowed"
* Connection #0 to host localhost left intact
}%

hawkw added 3 commits December 4, 2024 10:44
For example:

```console
eliza@noctis ~/Code/dropshot $ cargo run --example basic &
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/examples/basic`
Dec 04 18:51:47.826 INFO listening, local_addr: 127.0.0.1:41751

eliza@noctis ~/Code/dropshot $ curl -v http://localhost:41751/counter -X DELETE
* Host localhost:41751 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:41751...
* connect to ::1 port 41751 from ::1 port 53992 failed: Connection refused
*   Trying 127.0.0.1:41751...
* Connected to localhost (127.0.0.1) port 41751
* using HTTP/1.x
> DELETE /counter HTTP/1.1
> Host: localhost:41751
> User-Agent: curl/8.11.0
> Accept: */*
>
* Request completely sent off
Dec 04 18:59:14.913 INFO accepted connection, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751
Dec 04 18:59:14.917 INFO request completed, error_message_external: Method Not Allowed, error_message_internal: Method Not Allowed, latency_us: 834, response_code: 405, uri: /counter, method: DELETE, req_id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793, remote_addr: 127.0.0.1:35084, local_addr: 127.0.0.1:41751
< HTTP/1.1 405 Method Not Allowed
< allow: GET
< allow: PUT
< content-type: application/json
< x-request-id: e1d14d57-ca67-49cc-84b0-d1dc1e0e5793
< content-length: 93
< date: Wed, 04 Dec 2024 18:59:14 GMT
<
{
  "request_id": "e1d14d57-ca67-49cc-84b0-d1dc1e0e5793",
  "message": "Method Not Allowed"
* Connection #0 to host localhost left intact
}%

```
@hawkw hawkw requested review from davepacheco and ahl December 4, 2024 19:26
/// allocation containing the actual data. Thus, we store an
/// `Option<Box<HeaderMap>>` here, so that the empty case is just a null
/// pointer.
pub headers: Option<Box<http::HeaderMap>>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boxing this resolves a Clippy lint about the error type being too large. I figured we shouldn't just disable that lint, since it would presumably also trigger on user code that returns an HttpError, and it seems sad to have an API that results in lints in consumers if they don't go out of their way to disable them.

I also considered making the field private, and requiring it be accessed via methods, since the Option<Box<...>> makes it a bit awkward. However, that would prevent consumers of the API from manually constructing HttpErrors by initializing the other fields, which I've seen a lot of in Omicron, so I figured it was better to just make it public.

Comment on lines +361 to +365
/// Unlike [`HttpError::set_header`], this method takes `self` by value,
/// allowing it to be chained to form an expression that returns an
/// `HttpError`. However, because this takes `self` by value, returning an
/// error for an invalid header name or value will discard the `HttpError`.
/// To avoid this, use [`HttpError::set_header`] instead.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit awkward. Another option could be to make the method take HeaderName and HeaderValue only, instead of types that can be converted into them; then, it could be infallible, instead of potentially eating the HttpError.

I did it this way because it's similar to the http::Response::builder() API, and allows users to just pass strings as the header name/value, rather than having to convert them in user code. But, returning a Result and maybe throwing away the error is also unfortunate. I could be convinced to change this --- what do y'all think?

hawkw and others added 2 commits December 4, 2024 12:59
@hawkw hawkw requested a review from ahl December 4, 2024 21:12
@hawkw hawkw force-pushed the eliza/error-headers branch from f875822 to d69f141 Compare December 4, 2024 21:52
Co-authored-by: Adam Leventhal <[email protected]>
@hawkw hawkw merged commit 11a8838 into main Dec 4, 2024
10 checks passed
@hawkw hawkw deleted the eliza/error-headers branch December 4, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants